-
Notifications
You must be signed in to change notification settings - Fork 695
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prism version conflict #546
Conversation
@sslotsky -- If your deck is in a public repo, can you point me to it? If not, can you create a minimal repository that exposes the issue? We should be able to force a resolution correctly, but I need to see the full tree + build configuration to better understand what's going on. Looking at your links:
@kenwheeler @kitten -- could we use either of these? Would we want to?
Once we have a clearer reproduction of the error, we can all chat out best path forward... |
Hi! Do Spectacle's builds always use Babel? @mAAdhaTTah took the time to think about this and ended up writing a babel plugin for this: https://www.npmjs.com/package/babel-plugin-prismjs Maybe you could take a look at it and see if it can generate an appropriate file taking languages dependencies into account automatically. |
@ryan-roemer Yes, the The intention was definitely to solve this problem with the babel plugin, although I think you might need to use it both in spectacle & react-live (and possibly component-playground, if you're using it there too). The autoloader might also work, but it would require adding a build step in the project to copy all the components into the dist directory and setting the autoloader path at runtime. |
@ryan-roemer thanks for jumping on this so quickly and for bringing in the Prism folks! I have a minimal repo here that has the problem. Much appreciated. |
@mAAdhaTTah @Golmote -- Thanks. We do use babel and everything starts at @sslotsky -- I'm slammed today, but having a short repository is invaluable to us. We'll dig into this, verify the error, then assess in code what's actually going wrong, and then most likely work in some of the solutions mentioned above. |
🙇♂️ 🙏 🙌 |
@ryan-roemer Great! And we're here to help if you need anything. The babel plugin is brand new, so if you run into issues, let us know. I use Spectacle myself, so happy to do what I can. |
Error is verified. Will try to find time this week to dig in more, but I'm on vacation half the week, so may be after... |
Understood. Any way I can help? |
If someone wants to throw up a WIP / POC PR that could potentially work, that may aide our efforts. But if we're changing two projects simultaneously the infrastructure stuff might get complicated -- I'm pretty familiar with this type of situation and we've even written a dev tool So |
@mAAdhaTTah I actually took a crack at this on the react-live repo and did run into some trouble trying to run the demo app, which is SSR with next.js. Do you have any examples of using the babel plugin with a React SSR app? I think |
@sslotsky -- I haven't worked much in the Thanks for the info -- we'll start there first rather than spectacle itself. |
@sslotsky SSR shouldn't really impact things. All the babel plugin does is ensure all the dependencies are loaded in the correct order. Prism should otherwise work fine on the server; the |
@mAAdhaTTah I found that if I just naively follow the example on the babel plugin's README page, I get an error from I tried to move the //componentWillMount() {
// const html = prism(normalizeCode(this.props.code), this.props.language)
// this.setState({ html })
//}
componentDidMount() {
Prism.highlightAll();
const html = prism(normalizeCode(this.props.code), this.props.language)
this.setState({ html })
this.recordChange(this.getPlain())
this.undoTimestamp = 0 // Reset timestamp
} The import Prism from 'prismjs';
const prism = (code, language = 'jsx') => Prism.highlight(code, Prism.languages[language]) With this change I get an error in the Hopefully my mistake is simple? |
|
Yeah, PrismJS itself assumes a browser(-like) environment. That's not really specific to the babel plugin. The plugin configuration needs to be in an array: {
"plugins": [
["prismjs", {
"languages": ["clike", "javascript", "markup", "jsx", "reason"],
"plugins": ["line-numbers"],
"theme": "twilight",
"css": true
}]
]
} More generically speaking, your {
"plugins": [
["prismjs", config]
]
} |
Do you have a branch / fork I can look at? |
When react-live itself is built, it sticks with the This also requires consumers to run babel over their entire dependency chain, or at least any dependency that has Prism as a dependency. Is that a reasonable expectation for consumers? Also, right now, |
Speaking only for myself as a consumer, I think that the extra step of configuring the babel plugin might fall into the annoying-but-tolerable category, as long as the requirements and steps are highly discoverable. A way to automate as much of it as possible would be preferred, or at least have something make a lot of noise if the configuration is missing. |
Documentation in all 3 packages, plus default + links in the changelog should cover it. Maybe even update the starter / generator packages. Yeah, I'll add validation to the babel plugin settings though. It should yell at you. Those 3 packages will have to remove their current Prism imports, so the consumer can configure it completely. This might be a breaking change for those packages; not sure if there's any way around that 😕. |
I've pinned prismjs to 1.6.0 in react-live and bumped the version as a stopgap for the moment. @sslotsky I agree that a config would be annoying and I'd worry that forcing users to setup a babel plugin creates friction in what should be a relatively painless experience otherwise. NB. Some surprising behaviour when using the hub cli - I intended to reference the issue in the PR but instead hub converted the issue into a PR. I didn't even know that was a thing! Sorry everyone! We can open a new issue to continue the discussion if preferred. |
@imranolas thanks! Question: willing pinning prism to 1.6.0 allow me to use ReasonML syntax? I have some concerns that whichever Prism loading code that gets executed first/last could take precedent in terms of which languages are actually loaded. |
@sslotsky No problem. I've tested it against your minimal repro and it appears to work but I hadn't considered the issue could be loading precedence. Hopefully this is stable for now until we can find the right solution. |
@sslotsky -- It works provided no other packages you add have a But from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢 🤕
Prism was nailed down to 1.6.0 due to #512, but spectacle still depends on
react-live
which has a dependency on Prism 1.14.0This version conflict leaves me unable to show a code slide in ReasonML. I can't help but wonder if it would be better to just load the languages in the way that the Prism team recommends PrismJS/prism#1394